Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use message pack for project.razor.* configuration file #9270

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

DustinCampbell
Copy link
Member

This change uses the message pack formatters to serialize and deserialize the project configuration file. Of particular note, I added support to the message pack formatters for "skimming". The idea is that, when deserializing tag helpers, we look at the checksum first and determine whether that tag helper is in our cache. If it is, partially deserialize the tag helper by skimming through the byte stream. While skipping, we ensure that the SerializerCachingOptions are populated correctly with cached streams and MetadataCollections. In addition, because we're no longer serializing the file as "json", I've changed the file extension to ".bin".

This change allows message pack formatters to support "skimming" or
partially deserializing objects. This is needed to allow quickly
skipping through tag helpers that are already cached by checksum
without deserializing them. Skimming ensures that the
`SerializerCachingOptions` are correctly populated.
@DustinCampbell DustinCampbell requested a review from a team as a code owner September 11, 2023 23:38
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and exciting results.

My only question is why Skim uses ReadArrayHeader and not ReadArrayHeaderAndVerify (and whether "Skim" is the right name, and whether ".bin" is the right file extension 😛)

@DustinCampbell
Copy link
Member Author

My only question is why Skim uses ReadArrayHeader and not ReadArrayHeaderAndVerify

I looked through and didn't spot anyplace where Skim(...) didn't use ReadArrayHeaderAndVerify where the number of elements is know. Do you have a pointer to the code where you noticed this?

@DustinCampbell
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@davidwengier
Copy link
Contributor

I looked through and didn't spot anyplace where Skim(...) didn't use ReadArrayHeaderAndVerify where the number of elements is know. Do you have a pointer to the code where you noticed this?

I saw this and figured it could be ReadArrayHeaderAndVerify(checksums.Length), then saw the others and thought it must be deliberate. Now that I've had a better look, I think that might be the only place it could apply.

@DustinCampbell
Copy link
Member Author

I saw this and figured it could be ReadArrayHeaderAndVerify(checksums.Length), then saw the others and thought it must be deliberate. Now that I've had a better look, I think that might be the only place it could apply.

Good catch! Very good idea to use ReadArrayHeaderAndVerify(...) and avoid a Debug.Assert.

@DustinCampbell DustinCampbell merged commit c01e4d8 into dotnet:main Sep 12, 2023
12 checks passed
@DustinCampbell DustinCampbell deleted the message-pack-config-file branch September 12, 2023 22:46
@ghost ghost added this to the Next milestone Sep 12, 2023
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
phil-allen-msft added a commit to dotnet/roslyn that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants